refactor(utils): command helper #1113
Conversation
Co-authored-by: Matěj Chalk <[email protected]>
Co-authored-by: Matěj Chalk <[email protected]>
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared current commit e512344 with previous commit 33714e2. 🕵️ See full comparison in Code PushUp portal 🔍 🏷️ Categories
👍 1 group improved, 👎 3 groups regressed, 👍 2 audits improved, 👎 9 audits regressed, 12 audits changed without impacting score🗃️ Groups
17 other groups are unchanged. 🛡️ Audits
587 other audits are unchanged. |
github-actions
bot
added
🧩 core
🧩 nx-plugin
🧩 eslint-plugin
🧩 coverage-plugin
🧩 js-packages-plugin
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ing or encoding Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…nto refactor/utils/command-helper
| return new Promise((resolve, reject) => { | ||
| // shell:true tells Windows to use shell command for spawning a child process | ||
| const process = spawn(command, args, { cwd, shell: true }); | ||
| const spawnedProcess = spawn(command, args ?? [], { |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 days ago
To fix this, we should not use { shell: true } with arguments provided directly by untrusted/library input. Instead, we should use { shell: false } (the default), allowing spawn to pass the command and arguments directly to the process without interpretation by the shell. If there are legitimate use cases requiring shell features (such as pipes, I/O redirection), then inputs embedded in the shell command line must be safely escaped using a library like shell-quote. But for generic process execution, switching to non-shell mode is safest.
Thus:
- In packages/nx-plugin/src/internal/execute-process.ts, at line 175, remove or change
shell: trueto either omit it or setshell: false. - No other code needs changing, unless shell features are known to be required for all usage (which is not demonstrated here).
Alternatively, if some processes require shell interpretation and others do not, expose { shell: true/false } as an explicit configurable option, and document the dangers, but set default to false.
| @@ -173,7 +173,6 @@ | ||
| return new Promise((resolve, reject) => { | ||
| // shell:true tells Windows to use shell command for spawning a child process | ||
| const spawnedProcess = spawn(command, args ?? [], { | ||
| shell: true, | ||
| windowsHide: true, | ||
| ...options, | ||
| }) as ChildProcessByStdio<Writable, Readable, Readable>; |
There was a problem hiding this comment.
Should we add a comment that we need it for windows (I'm not sure what exactly the reason was :D)
| command: string; | ||
| args?: string[]; | ||
| observer?: ProcessObserver; | ||
| verbose?: boolean; |
There was a problem hiding this comment.
Adding verbose: isVerbose() to each usage is quite error-prone. We lose the advantage of setting CP_VERBOSE once and then having all process executions automatically logged or not consistently. 😬
However, I understand you're trying to make the implementation as isolated as possible, to make it easier to maintain a copy.
I think there may be a better solution. CJS can't statically import ESM with require 1, but CJS can dynamically import ESM. For many synchronous utility functions, this would be a little inconvenient - code would need to become async because of await import('@code-pushup/utils') - but executeProcess is already asynchronous, so there isn't really any downside.
const { executeProcess } = await import('@code-pushup/utils');
await executeProcess(...);For convenience, you could implement a simple wrapper to deduplicate usage across @code-pushup/nx-plugin:
// somewhere in nx-plugin's internal utils
// same function signature => exact same usage
export async function executeProcess(cfg: ProcessConfig): Promise<ProcessResult> {
// CJS-compatible import
const utils = await import('@code-pushup/utils');
// no duplicated logic, call original code
return utils.executeProcess(cfg);
}Experiments with importing ESM into CJS
// utils.mjs
// top-level await to break require(esm) in modern Node
await new Promise(resolve => {
setTimeout(resolve, 500);
});
// ESM syntax
export function greet(name = 'world') {
console.log(`Hello, ${name}!`);
}// nx-plugin.cjs
// ❌ THIS DOESN'T WORK (ERR_REQUIRE_ESM or ERR_REQUIRE_ASYNC_MODULE)
const { greet } = require('./utils.mjs');
greet('Nx plugin');// nx-plugin.cjs
// ✅ THIS WORKS
import('./utils.mjs').then(({ greet }) => {
greet('Nx plugin');
});Footnotes
-
Technically, CJS can statically
requireESM if running a newer Node version (22+, I believe) and there's no top-levelawaitanywhere in the imported module. Unfortunately, because we want to maintain compatibility for users running older Node versions, the minimum version is probably a deal-breaker for us at the moment. ↩
This PR includes:
command.tsexecuteProgress- removed hard dependencies to other functions so it is easier to use innx-plugincommand.tsandexecuteProcess.tsintonx-pluginproject and marked them